[Woo POS][Local Catalog] Save parsed full catalog into database #16087

joshheald · 2025-09-04T13:22:31Z

Description

This PR adds a simple persistence service to save the full catalog to the database, after it's downloaded.

Deletes all data from the database
Inserts Products, Variations, Attributes, and Images, ignoring any conflicts
If that succeeds, commits the transaction, otherwise rolls back and returns to the pre-deletion database.

Ignoring conflicts

On Large fun testing, I found that all my attempts to persist the data would fail with a primary key conflict.

You'll see that 4977 products are downloaded, but only 4976 persisted – I think that we get one of them twice.

For a large import, I think our best bet is to ignore conflicts. That gives a best-effort at loading the catalog, but if the data is duplicated we'll just keep the first one we get and continue with the import. An alternative would be to specify replace.

For images, this means that we only save one copy of each, with a direct relationship to the first product that we imported it for. Most products and variations don't have images with this implementation, but I'll fix that schema issue in a future PR. We either need a many-to-many join table, or to make copies with synthetic keys as we do for attributes.

Steps to reproduce

Check unit tests pass.

Testing information

This isn't used in the app yet. I added a button to test it – see https://github.com/woocommerce/woocommerce-ios/compare/woomob-1093-woo-poslocal-catalog-save-parsed-full-catalog-into-database-with-button for the branch with that on top.

When you persist everything, log messages tell you what was downloaded, and what was persisted. The detailed log in the message below is the most accurate, as it reads its numbers out of the db.

Screenshots

This is from Large fun testing. There genuinely are only 100 images in the data set, and only 101 on the site – they're shared between products and variations. That's not properly supported by our schema yet, but I'll fix it in a future PR.

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Add persistence helper in Yosemite

wpmobilebot · 2025-09-04T13:31:50Z

📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS Prototype
	Build Number	`pr16087-76b396b`
	Version	`23.2`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`76b396b`
	Installation URL	1ulvea1j4dft0

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

jaclync

Looks good and tests well! Sounds good to update the relationship handling separately.

You'll see that 4977 products are downloaded, but only 4976 persisted – I think that we get one of them twice.

This is quite strange, I saw 4977 products in wp-admin and spent a bit of time checking what's going on by comparing the product IDs in the database and from the API / wp-admin. It turned out product ID 17300 appeared twice in both the API and wp-admin:

the end of page 5 (20 products per page)	the beginning of page 6

It feels like some bug in core? wp-admin fetches 20 per page and the API fetches 100 per page, might not be due to pagination. But at least we know it's not something on the client side, and we will keep potential duplicates in mind.

jaclync · 2025-09-05T05:17:07Z

Modules/Sources/Yosemite/Tools/POS/POSCatalogFullSyncService.swift

 import class Networking.AlamofireNetwork
 import class Networking.POSCatalogSyncRemote
 import CocoaLumberjackSwift
+import Storage


super nit: maybe can import just POSCatalogPersistenceServiceProtocol and POSCatalogPersistenceService instead of the whole Storage?

It's actually for GRDBManagerProtocol, the persistence service is in Yosemite anyway so doesn't need to be imported.

I'm not sure that granular imports help us much here. AIUI, we do it from the main app to make it easier to split POS to a new app, if we ever do that. Storage is much simpler, and we implicitly depend on most of it via the storage model typealiases.

Perhaps a different question: Should GRDB be in a new module of its own? Maybe it would be best to do that now, in a new PR but before we add more to it. WDYT?

I'm not sure that granular imports help us much here. AIUI, we do it from the main app to make it easier to split POS to a new app, if we ever do that. Storage is much simpler, and we implicitly depend on most of it via the storage model typealiases.

My understanding for granular imports is so that it's clear which (minimum) dependencies are required for the file. It's more useful when the imported module is bigger though, like Yosemite. For Storage I think it's fine, just a nit preference.

Perhaps a different question: Should GRDB be in a new module of its own? Maybe it would be best to do that now, in a new PR but before we add more to it. WDYT?

Were you thinking of keeping GRDB code in a separate module, and Storage imports it for its usage with potential abstraction in the interface with Yosemite? Was it because the GRDB code might get complex, or any other reasons? Right now, I think it's fine for GRDB to be within Storage as an implementation of the protocols. Would like to hear any strong reasons for moving it to a separate module.

Were you thinking of keeping GRDB code in a separate module, and Storage imports it for its usage with potential abstraction in the interface with Yosemite? Was it because the GRDB code might get complex, or any other reasons? Right now, I think it's fine for GRDB to be within Storage as an implementation of the protocols. Would like to hear any strong reasons for moving it to a separate module.

No, I was thinking a new sibling module for Storage, e.g. GRDBStorage (bad name, but for discussion).

Our new persisted models don't rely on anything pre-existing in Storage. Now's the easiest time to separate them so that we don't end up with anything cutting across both types of storage.

By putting them in a separate module, we could call them Product, ProductVariation etc, at least within GRDBStorage. If we split POS into a separate app, we might be able to leave Storage behind (unlikely, but not impossible.)

I don't have super-strong arguments beyond that. Keeping it in Storage is fine... but other than the name, and saving a little effort adding a module, there's no particular reason it has to be in there.

I see, so the current Storage is more like CoreDataStorage. Makes sense to me. We could put it up for discussion and fit this after the main tasks are done.

From development so far, it's not a strong need, but it might be great to have more abstraction on the interface between the (GRDB)Storage and Yosemite layers.

Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift

jaclync · 2025-09-05T05:28:44Z

Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift

+    private(set) var replaceAllCatalogDataCallCount = 0
+    private(set) var lastPersistedCatalog: POSCatalog?
+    private(set) var lastPersistedSiteID: Int64?


I'm guessing test cases that assert on these mock persistence service variables will be added in a future PR?

jaclync · 2025-09-05T05:32:38Z

Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift


        // When
-        let service = POSCatalogFullSyncService(syncRemote: mockSyncRemote, batchSize: customBatchSize)
+        let service = POSCatalogFullSyncService(syncRemote: mockSyncRemote, batchSize: customBatchSize, persistenceService: mockPersistence)


nit: could we use mockPersistenceService here, instead of a separate mockPersistence?

Yes, good catch. I think this was from some tidying up or something.

jaclync · 2025-09-05T05:42:14Z

Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift

+            // One or both images should be saved (depending on conflict resolution)
+            #expect(imageCount >= 1)
+            #expect(imageCount <= 2)


interesting, this would be resolved when we support either many-to-many relationship with a join table or a composite primary key right? as mentioned in the description:

For images, this means that we only save one copy of each, with a direct relationship to the first product that we imported it for. Most products and variations don't have images with this implementation, but I'll fix that schema issue in a future PR. We either need a many-to-many join table, or to make copies with synthetic keys as we do for attributes.

Yes.

Right now, only one will be saved, because we use the imageID from the network as the primary key id on the table. However, we don't have a join table, so it'll only belongTo the first product which it's added for – the second won't have an image. That's the ignore conflict resolution.

We can either make it many to many with a join table, or make copies for every product with a synthetic primary key.

If we make it many to many, then one image would still be saved. If we make copies, two images would be saved.

For now I'll simplify this test to check for exactly one.

jaclync · 2025-09-05T05:44:03Z

Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift

+        }
+    }
+
+    @Test func replaceAllCatalogData_removes_related_images_and_attributes() async throws {


nice! do we also want to add similar setup/asserts for variation?

Sure: b1ca363

Co-authored-by: Jaclyn Chen <[email protected]>

…d-full-catalog-into-database

joshheald added 6 commits September 3, 2025 16:42

Ensure variation images are deleted with cascade

faeb1c5

Add persistence helper in Yosemite

b12bbec

Add persistence helper in Yosemite

Use a single transaction to write all entities

442f2cc

Test saving the catalog to the database

d0b1e6b

Periphery fixes and ignore

f2b005d

Clear data in replaceAllCatalogData transaction

ae7aab8

joshheald added this to the 23.3 milestone Sep 4, 2025

joshheald added type: task An internally driven task. status: feature-flagged Behind a feature flag. Milestone is not strongly held. labels Sep 4, 2025

joshheald requested a review from jaclync September 4, 2025 13:28

jaclync approved these changes Sep 5, 2025

View reviewed changes

joshheald and others added 5 commits September 5, 2025 10:53

Remove commented out deleteOne ideal code

bad8441

Co-authored-by: Jaclyn Chen <[email protected]>

Use mock from property

d3a7978

Make image test match current (flawed) implementation

ce9afe1

Add test for variation image/attribute removal

501e954

Merge branch 'trunk' into woomob-1093-woo-poslocal-catalog-save-parse…

76b396b

…d-full-catalog-into-database

joshheald enabled auto-merge September 5, 2025 10:17

joshheald merged commit 7688160 into trunk Sep 5, 2025
14 checks passed

joshheald deleted the woomob-1093-woo-poslocal-catalog-save-parsed-full-catalog-into-database branch September 5, 2025 10:39

[Woo POS][Local Catalog] Save parsed full catalog into database #16087

[Woo POS][Local Catalog] Save parsed full catalog into database #16087

Uh oh!

Conversation

joshheald commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Ignoring conflicts

Steps to reproduce

Testing information

Screenshots

Uh oh!

wpmobilebot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joshheald commented Sep 4, 2025 •

edited

Loading

wpmobilebot commented Sep 4, 2025 •

edited

Loading